[FLINK-36836][Autoscaler] Supports config the upper and lower limits of target utilization #921

huyuanfeng2018 · 2024-12-04T11:24:50Z

What is the purpose of the change

detail: FLINK-36836

Brief change log

Introduce two new parameters job.autoscaler.utilization.min and job.autoscaler.utilization.max to determine the upper and lower limits of utilization

Verifying this change

Change the previous test cases related to job.autoscaler.target.utilization.boundary to job.autoscaler.utilization.min and job.autoscaler.utilization.max
add test ScalingExecutorTest#testUtilizationBoundariesAndUtilizationMinMaxCompatibility Verify compatibility of parameters before and after

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changes to the CustomResourceDescriptors: (yes / no)
Core observer or reconciler logic that is regularly executed: (yes / no)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

1996fanrui

Thanks @huyuanfeng2018 for the PR.

Although it's just a draft PR, I checked the changes of AutoScalerOptions, something is unexpected from me. So I would give some feedbacks in advance.

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java

huyuanfeng2018 · 2024-12-05T11:20:54Z

Thanks @huyuanfeng2018 for the PR.

Although it's just a draft PR, I checked the changes of AutoScalerOptions, something is unexpected from me. So I would give some feedbacks in advance.

Thanks for the quick suggestion!

I have made the suggested modifications and added the test case, PTAL.

flink-autoscaler/src/test/java/org/apache/flink/autoscaler/ScalingExecutorTest.java

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricEvaluator.java

...ator/src/test/java/org/apache/flink/kubernetes/operator/validation/DefaultValidatorTest.java

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricEvaluator.java

1996fanrui

Thanks @huyuanfeng2018 for the update!

Overall LGTM, I only left 2 minor comments, please take a look in your free time, thanks~

...operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java

...ator/src/test/java/org/apache/flink/kubernetes/operator/validation/DefaultValidatorTest.java

mxm

Thanks for the PR!

mxm · 2024-12-11T08:28:16Z

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricEvaluator.java

+            if (conf.getOptional(UTILIZATION_MAX).isPresent()
+                    || conf.getOptional(UTILIZATION_MIN).isPresent()
+                    || conf.getOptional(TARGET_UTILIZATION_BOUNDARY).isEmpty()) {
+                upperUtilization = conf.get(UTILIZATION_MAX);
+                lowerUtilization = conf.get(UTILIZATION_MIN);


I'm wondering whether the logic should be the following?

Suggested change

if (conf.getOptional(UTILIZATION_MAX).isPresent()

|| conf.getOptional(UTILIZATION_MIN).isPresent()

|| conf.getOptional(TARGET_UTILIZATION_BOUNDARY).isEmpty()) {

upperUtilization = conf.get(UTILIZATION_MAX);

lowerUtilization = conf.get(UTILIZATION_MIN);

if (conf.getOptional(UTILIZATION_MAX).isPresent()

&& conf.getOptional(UTILIZATION_MIN).isPresent()) {

upperUtilization = conf.get(UTILIZATION_MAX);

lowerUtilization = conf.get(UTILIZATION_MIN);

I think as long as one of UTILIZATION_MAX and UTILIZATION_MIN is set, it means that the user is already using the new config.

At the same time, if the user does not set TARGET_UTILIZATION_BOUNDARY, we still use the new config

I think we should not hard-code the defaults in the MIN_MAX options but it should be dynamically computed based on the target.

For example if the users sets target to 0.4 and min to 0.1, then it doesn't make sense to have the default max on 1.. Previously the boundary was 0.4 by default I think we should preserve this

I don't know if there is something wrong with my understanding:

Should we keep the boundary and then the max and min values are derived by boundary + target and target - boundary by default? Unless the user explicitly sets min max?

If that's the case, I think it's reasonable.

But in this case, should we change the min max parameter like to utilization.lower.boundary and utilization.upper.boundary ?
Then max = upper.boundary+ target , min = target- upper.boundary

As @mxm suggested I think the logic should be enabled if either MIN or MAX is defined. If only one of them is defined then we can compute the default for the other one from the target utilization +- boundary.

I think we should not have default values for the min/max options

As @mxm suggested I think the logic should be enabled if either MIN or MAX is defined

@gyfora Did you mean, if BOTH min and max are defined, the new logic should be enabled? If only one of them is defined, then we compute the other one using the configured target utilization and the boundary. I think that makes sense 👍

+1 for not having static defaults for the new options because that would be confusing to users.

Thanks @mxm @gyfora for review.

compute the default for the other one from the target utilization +- boundary.

+1 for this .

I've modified and added compatibility test cases.

PTAL~

mxm · 2024-12-11T08:29:33Z

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java

-    public static final ConfigOption<Double> TARGET_UTILIZATION =
-            autoScalerConfig("target.utilization")
+    public static final ConfigOption<Double> UTILIZATION_TARGET =
+            autoScalerConfig("utilization.target")


Do we have to change this well-established key? This might confuse users, even though we have a fallback.

I understand its cleaner, but perhaps we can simply add the new keys and leave this unchanged?

I understand its cleaner, but perhaps we can simply add the new keys and leave this unchanged?
Do we have to change this well-established key? This might confuse users, even though we have a fallback.

I think methed withFallbackKeys good enough
WDYT?

I think in the long term it's preferable to have consistent well named keys. I think the fallback keys should be enough. I would expect that users will gravitate towards the new min/max options and at that point this will make a lot of sense for everyone.

huyuanfeng2018 · 2024-12-11T12:21:34Z

Thanks @1996fanrui @mxm review.

Changes and replies have been made based on comments，PTAL.

thanks~

1996fanrui

Hi @huyuanfeng2018 , thanks for the update!

I left 2 comments for the last commit, and they are related to #921 (comment) (Compatibility issues), please @gyfora and @mxm help double confirm, thanks a lot.

1996fanrui · 2025-01-03T08:48:41Z

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java

+    public static final ConfigOption<Double> UTILIZATION_MAX =
+            autoScalerConfig("utilization.max")
+                    .doubleType()
+                    .noDefaultValue()
+                    .withFallbackKeys(oldOperatorConfigKey("utilization.max"))
+                    .withDescription("Max vertex utilization");
+
+    public static final ConfigOption<Double> UTILIZATION_MIN =
+            autoScalerConfig("utilization.min")
+                    .doubleType()
+                    .noDefaultValue()
+                    .withFallbackKeys(oldOperatorConfigKey("utilization.min"))
+                    .withDescription("Min vertex utilization");


These 2 options have no default value, I'm not sure whether it's expected.

If the target.utilization.boundary is removed in the future, how do we handle the default value?

Usually, when a new option is introduced, a default value is also designed. When deprecated options are removed in subsequent versions, we only need to remove unnecessary compatibility code.

Well, maybe retaining the target.utilization.boundary parameter is also a feasible solution,like latest code changes, I don't know if my understanding is consistent with @gyfora and @mxm.

As I understand, we will only keep the following three options in the end, and make it compatible with job.autoscaler.target.utilization.boundary before it is deprecated.

job.autoscaler.utilization.target job.autoscaler.utilization.min job.autoscaler.utilization.max

Do you mean we keep all 4 options in the end?

I think we should remove the boundary at the end (deprecate now), but still min/max should not have fixed defaults as the default would be derived from the current target.

1996fanrui · 2025-01-03T08:53:29Z

flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java

-    public static final ConfigOption<Double> TARGET_UTILIZATION_BOUNDARY =
-            autoScalerConfig("target.utilization.boundary")
+    public static final ConfigOption<Double> UTILIZATION_TARGET_BOUNDARY =
+            autoScalerConfig("utilization.target.boundary")


This option is deprecated, do we need to add a @Deprecated annotation here?

Also, IIUC, we don't need to rename the option key name since it's deprecated. It means new users don't need it and we don't maintain it in the future. (Compatibility needs to be considered only in the short term.)

If target.utilization.boundary will not be deprecated in the future，Is it reasonable?

we should not rename this option also, the name you set is incorrect anyways. Keys should not be prefixes of each other (not yaml compliant)

huyuanfeng2018 · 2025-01-08T02:02:47Z

Thanks @1996fanrui @gyfora for the reply

Well, let me summarize our conclusion,

we need to remove the boundary parameter in the future (deprecate now), So there is no need to rename it now
default min/max need to be derived from target

Then the logic for determining the default value of min/max is ：

now（boundary exist） : max = upper.boundary+ target , min = target- upper.boundary

future（boundary remove） : max = 0.3 + target , min = target- 0.3, 0.3 is a static value

I don’t know if there is anything wrong with my understanding.

gyfora · 2025-01-08T07:02:05Z

Thanks @1996fanrui @gyfora for the reply

Well, let me summarize our conclusion,

we need to remove the boundary parameter in the future (deprecate now), So there is no need to rename it now

default min/max need to be derived from target

Then the logic for determining the default value of min/max is ：

now（boundary exist） : max = upper.boundary+ target , min = target- upper.boundary

future（boundary remove） : max = 0.3 + target , min = target- 0.3, 0.3 is a static value

I don’t know if there is anything wrong with my understanding.

That sounds fully correct :)

1996fanrui

Thanks for the update, LGTM assuming the CI is green.

huyuanfeng2018 · 2025-01-10T02:20:41Z

Thanks for the update, LGTM assuming the CI is green.

Thanks for your quick reply

…of target utilization

…ated by `utilization.target.boundary`

huyuanfeng2018 marked this pull request as draft December 4, 2024 11:24

1996fanrui reviewed Dec 5, 2024

View reviewed changes

huyuanfeng2018 force-pushed the FLINK-36836 branch from 4368ebe to eba5b27 Compare December 5, 2024 11:12

huyuanfeng2018 marked this pull request as ready for review December 5, 2024 11:21

1996fanrui self-assigned this Dec 5, 2024

1996fanrui reviewed Dec 6, 2024

View reviewed changes

huyuanfeng2018 force-pushed the FLINK-36836 branch 2 times, most recently from 02c01aa to 8907920 Compare December 8, 2024 09:04

1996fanrui reviewed Dec 10, 2024

View reviewed changes

...operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java Outdated Show resolved Hide resolved

...ator/src/test/java/org/apache/flink/kubernetes/operator/validation/DefaultValidatorTest.java Outdated Show resolved Hide resolved

mxm reviewed Dec 11, 2024

View reviewed changes

huyuanfeng2018 force-pushed the FLINK-36836 branch from 8907920 to 65c53e7 Compare December 11, 2024 12:17

1996fanrui approved these changes Dec 12, 2024

View reviewed changes

1996fanrui requested changes Jan 3, 2025

View reviewed changes

huyuanfeng2018 force-pushed the FLINK-36836 branch 2 times, most recently from 21bc13c to e57f4b9 Compare January 10, 2025 02:12

1996fanrui approved these changes Jan 10, 2025

View reviewed changes

gyfora approved these changes Jan 13, 2025

View reviewed changes

gyfora mentioned this pull request Jan 13, 2025

[FLINK-36863][autoscaler] Use the maximum parallelism in the past scale-down.interval window when scaling down #922

Merged

huyuanfeng added 2 commits January 14, 2025 09:53

[FLINK-36836][Autoscaler] Supports config the upper and lower limits …

4da1b30

…of target utilization

[FLINK-36836][Autoscaler] Remove the default value of max min, calcul…

08315cd

…ated by `utilization.target.boundary`

huyuanfeng2018 force-pushed the FLINK-36836 branch from e57f4b9 to 08315cd Compare January 14, 2025 01:54

1996fanrui merged commit 66f7b42 into apache:main Jan 14, 2025
104 checks passed

huyuanfeng2018 deleted the FLINK-36836 branch January 14, 2025 02:28

[FLINK-36836][Autoscaler] Supports config the upper and lower limits of target utilization #921

[FLINK-36836][Autoscaler] Supports config the upper and lower limits of target utilization #921

Uh oh!

Conversation

huyuanfeng2018 commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

1996fanrui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huyuanfeng2018 commented Dec 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1996fanrui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huyuanfeng2018 commented Dec 11, 2024

Uh oh!

1996fanrui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huyuanfeng2018 commented Jan 8, 2025

huyuanfeng2018 commented Dec 4, 2024 •

edited

Loading